Skip to content

Improve Prebid auction diagnostics#671

Open
ChristianPavilonis wants to merge 6 commits into
mainfrom
feature/prebid-auction-diagnostics
Open

Improve Prebid auction diagnostics#671
ChristianPavilonis wants to merge 6 commits into
mainfrom
feature/prebid-auction-diagnostics

Conversation

@ChristianPavilonis
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis commented Apr 29, 2026 •
edited
Loading

Summary

  • Improve Prebid auction diagnostics so upstream HTTP failures surface status in /auction provider metadata, with capped body previews included only when Prebid debug mode is enabled.
  • Treat successful empty Prebid responses (204 No Content or empty 200) as nobid instead of provider error.
  • Remove Rubicon from the default client_side_bidders sample config so it is not enabled by default.

Changes

File Change
crates/trusted-server-core/src/integrations/prebid.rs Adds upstream HTTP status metadata for non-2xx responses, gates capped Prebid response body previews behind debug, treats empty successful responses as no-bid, uses PREBID_INTEGRATION_ID for provider response IDs, and adds tests for truncation/non-UTF-8 preview behavior.
crates/trusted-server-core/src/auction/orchestrator.rs Preserves provider launch/parse/conversion errors as provider response metadata using client-safe current-context display text, and adds tests for safe output and truncation.
trusted-server.toml Changes default client_side_bidders from ["rubicon"] to [].

Compatibility notes

  • provider_details may include launch-failure diagnostics before awaited provider responses. Bid selection is order-insensitive, but consumers should not rely on provider_details being ordered by response completion.

Closes

N/A — no issue filed.

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo test -p trusted-server-core prebid -- --nocapture; cargo test -p trusted-server-core auction -- --nocapture; targeted review-feedback tests for provider_error, prebid_body_preview, and parse_response_non_success

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses log macros per CLAUDE.md (template says tracing, but this repo standard is log)
  • New code has tests
  • No secrets or credentials committed

aram356
aram356 requested changes May 1, 2026
View reviewed changes
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs
Comment thread crates/trusted-server-core/src/integrations/prebid.rs
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs
Comment thread crates/trusted-server-core/src/integrations/prebid.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/prebid.rs Outdated
@aram356
Copy link
Copy Markdown
Collaborator

aram356 commented May 1, 2026

Summary

Richer /auction diagnostics and correct no-bid classification for empty Prebid responses are well-motivated and well-tested. One blocker: error-stack Report Debug formatting bleeds internal source paths into the public /auction JSON response, which contradicts the explicit policy in error.rs separating client-facing messages from server-only details.

Blocking

🔧 wrench

  • Information disclosure via format!("{error:?}"): provider_error_message ships error-stack Debug output (including at <file>:<line> entries) into the public /auction provider_details[].metadata.message. See inline. Switch to error.current_context().to_string() to keep the user-safe Display text but drop file/line/stack.

Non-blocking

🤔 thinking

  • Upstream body_preview proxied to public clients: The first 1000 chars of upstream Prebid error bodies are returned in the /auction response. Consider gating on self.config.debug (mirrors the existing trace-log gate) or a request-time header. See inline.
  • provider_details ordering changed: Launch failures now precede awaited responses in responses, whereas previously the list was ordered by completion. select_winning_bids is unaffected, but external consumers parsing provider_details see a different order. See inline.

♻️ refactor

  • Use PREBID_INTEGRATION_ID constant: New AuctionResponse::error("prebid", …) / ::no_bid("prebid", …) call sites should use the existing constant (crates/trusted-server-core/src/integrations/prebid.rs:36) instead of the literal. See inline.

🌱 seedling

  • Standardize diagnostic metadata across providers: Orchestrator-level error_type tagging is provider-agnostic, but only Prebid produces http_status / body_preview for non-success upstream responses. APS/GAM/etc. would be silent on the same failure mode. Worth a follow-up.

📌 out of scope

  • client_side_bidders = [] is a silent client-side capability change: The JS bundle still ships the rubicon adapter at build time (crates/js/lib/build-all.mjs:32 → DEFAULT_PREBID_ADAPTERS = 'rubicon'), but the runtime default in trusted-server.toml no longer enables it. Publishers copying the example config will lose rubicon client-side without an adapter-time signal. Worth a CHANGELOG / release note.

⛏ nitpick

  • Missing truncation tests: Neither prebid_body_preview (1000-char cap, String::from_utf8_lossy) nor provider_error_message (500-char cap) has a test pinning truncation/non-UTF-8 behavior. See inline.
  • format!("{error:?}") allocates before truncation: Long error chains materialize a multi-KB string before chars().take(500) discards the tail. Becomes a non-issue if the wrench above is fixed (Display output is much shorter).

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS (incl. 4 new prebid + 1 orchestrator)
  • js tests: PASS
  • format-typescript / format-docs / browser integration tests / CodeQL: PASS

@aram356 aram356 requested a review from prk-Jr May 3, 2026 22:19
@ChristianPavilonis ChristianPavilonis requested a review from aram356 May 4, 2026 14:28
aram356
aram356 requested changes May 10, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The diagnostic improvements are well-scoped and the prior round's findings are cleanly addressed (current-context truncation, debug-gated body_preview, PREBID_INTEGRATION_ID constant, regression tests for the 500/1000-char caps and attached-detail suppression). CI is green and the new tests pin the right boundaries.

The remaining concerns are about consistency of that boundary: a few error paths slip past the debug gate that the 4xx path now establishes. Inline comments below cover the three specific call sites; cross-cutting items are listed here.

Blocking

🔧 wrench

  • Parse-error message bypasses debug gate: parse_response returns serde-error detail + body byte length unconditionally, while the 4xx branch hides body content behind config.debug (crates/trusted-server-core/src/integrations/prebid.rs:1141).
  • warn! emits up to 1000 chars of upstream body without debug gate: previously trace-only; this is a logging behavior change worth gating in line with the metadata-path treatment (crates/trusted-server-core/src/integrations/prebid.rs:1112).
  • Launch-failure metadata exposes internal message strings: e.g. signing-key kid via RequestSigner::from_services(). provider_error_message is sound; the issue is at the call site choosing to emit current_context() verbatim for error_type=launch_failed (crates/trusted-server-core/src/auction/orchestrator.rs:385).

Non-blocking

🌱 seedling

  • Network-layer select() failures still drop providers from provider_responses (crates/trusted-server-core/src/auction/orchestrator.rs:481-486). The PR adds metadata for launch_failed, parse_response, and unsupported_response_body, but transport-level errors emerging from select() Err — and providers still pending after the deadline (backend_to_provider leftovers) — produce no entry. /auction clients can't distinguish "no bid" from "transport error" for those. Pre-existing, but the PR's stated intent is unified observability so it's a natural follow-up.

📌 out of scope

  • JS bundle still defaults to bundling the Rubicon adapter (crates/js/lib/build-all.mjs:32). With client_side_bidders = [] now the default in trusted-server.toml, the bundled Rubicon Prebid.js adapter is dead weight unless a publisher overrides the list. Either default TSJS_PREBID_ADAPTERS='' to match, or have build-all.mjs derive the adapter list from the merged TOML. Worth a follow-up issue.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS
  • integration / browser tests: PASS
  • CodeQL / format-typescript / format-docs: PASS

Comment thread crates/trusted-server-core/src/integrations/prebid.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/prebid.rs Outdated
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs Outdated
@ChristianPavilonis ChristianPavilonis requested review from aram356 and prk-Jr and removed request for prk-Jr May 11, 2026 16:06
prk-Jr
prk-Jr requested changes May 11, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The remaining review concern is in the Prebid error-body preview path. The public preview is capped, but the helper still performs uncapped UTF-8/lossy processing before the cap is applied, and it is invoked even when debug mode is off.

Blocking

🔧 wrench

  • Uncapped preview work before truncation: prebid_body_preview converts the entire upstream body with String::from_utf8_lossy(body) before taking the first 1000 chars, and the non-success response path calls it before checking config.debug. Large or invalid UTF-8 error bodies can still drive uncapped validation/allocation even when no preview will be exposed. See inline.

CI Status

  • GitHub checks: Analyze (javascript-typescript) PASS
  • Local fmt: PASS (cargo fmt --all -- --check)
  • Local targeted Rust tests: PASS (prebid_body_preview; provider_error_response)

Comment thread crates/trusted-server-core/src/integrations/prebid.rs Outdated
@ChristianPavilonis ChristianPavilonis force-pushed the feature/prebid-auction-diagnostics branch from 25eaca6 to cedc9cf Compare May 11, 2026 16:35
@ChristianPavilonis ChristianPavilonis requested a review from prk-Jr May 11, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aram356 aram356 Awaiting requested review from aram356

@prk-Jr prk-Jr Awaiting requested review from prk-Jr

Requested changes must be addressed to merge this pull request.

Assignees

@ChristianPavilonis ChristianPavilonis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ChristianPavilonis @aram356 @prk-Jr